Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(openapi): build script to setup ESM/CJS module exports + various fixes #10

Closed
wants to merge 4 commits into from
Closed

chore(openapi): build script to setup ESM/CJS module exports + various fixes #10

wants to merge 4 commits into from

Conversation

gfortaine
Copy link

@gfortaine gfortaine commented Jan 4, 2023 •
edited
Loading

1 PR to rule them all 😉 cc @schnerd @shun-shobon @ericlewis :

° Changelog

@gfortaine gfortaine changed the title chore(esm): enable ESM chore(node): enable ESM Jan 4, 2023
@gfortaine
Copy link
Author

Related openai/openai-node#45

@gfortaine gfortaine changed the title chore(node): enable ESM chore(node): enable ESM support Jan 4, 2023
@gfortaine gfortaine mentioned this pull request Jan 4, 2023
Closed
@gfortaine gfortaine changed the title chore(node): enable ESM support chore(openapi): use typescript generator w/ fetch Jan 17, 2023
@gfortaine gfortaine mentioned this pull request Jan 17, 2023
Closed
@ctjlewis
Copy link

ctjlewis commented Jan 19, 2023 •
edited
Loading

Isomorphic __dirname is tricky but can be done, you have to make guarantees about the file structure but it can be achieved with a polyfill and added as a banner to Esbuild. See: https://github.com/tsmodule/tsmodule/blob/master/src/specification/shim.ts

Nice work on this also.

schnerd
schnerd reviewed Jan 22, 2023
View reviewed changes
Copy link
Collaborator

@schnerd schnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really liking this change overall. Left a few comments and still need to do some testing on the final package before proceeding, but overall this is looking promising.

config.yaml Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
sdk-template-overrides/typescript/package.mustache Outdated
"prepublishOnly": "npm run prepare"
},
"dependencies": {
"@fortaine/fetch": "^6.2.2",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different than @vercel/fetch? Would prefer not to use this fork if we can avoid it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vercel/fetch doesn't work with nodejs native fetch at the time of writing. A PR has been made : vercel/fetch#76

@schnerd
Copy link
Collaborator

schnerd commented Jan 22, 2023

This new typescript generator is also listed as "Experimental" – do you know what's experimental about it? I'm a little hesitant to fully switch our production SDK over to something that is still marked as experimental. Have other well-known projects started using this generator?

@gfortaine
Copy link
Author

gfortaine commented Jan 22, 2023 •
edited
Loading

This new typescript generator is also listed as "Experimental" – do you know what's experimental about it? I'm a little hesitant to fully switch our production SDK over to something that is still marked as experimental. Have other well-known projects started using this generator?

@gfortaine
Copy link
Author

This new typescript generator is also listed as "Experimental" – do you know what's experimental about it? I'm a little hesitant to fully switch our production SDK over to something that is still marked as experimental. Have other well-known projects started using this generator?

By the way, we tried with typescript-fetch in the first place. However, it seems that there is a nasty bug when using oneOf

@gfortaine gfortaine force-pushed the master branch 4 times, most recently from efed3b4 to 9a89478 Compare January 24, 2023 23:59
@gfortaine gfortaine force-pushed the master branch 7 times, most recently from 02d3cec to 7d1c02f Compare January 30, 2023 02:23
@dqbd dqbd mentioned this pull request Feb 23, 2023
Closed
@gfortaine gfortaine changed the title chore(openapi): use typescript generator w/ fetch chore(openapi): build script to setup ESM/CJS module exports + various fixes Mar 5, 2023
@gfortaine
• chore(upgrade): enable ESM support
26808f8 
• chore(upgrade): get the version from the package.json w/o using require (prebuild script)
@gfortaine
chore(upgrade): add streamCompletion helper function (credits to @sch…
8643250 
…nerd & @rauschma)
This was referenced Mar 10, 2023
Open
Closed
@logankilpatrick
Copy link
Contributor

Now that we have the updated NodeSDK, I am going to close this PR. Please let me know if there's further discussion needed here and thanks for diving in on this originally!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schnerd schnerd schnerd left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gfortaine @ctjlewis @schnerd @logankilpatrick